Skip to content

Allow dynamic quota creation and removal#287

Open
QuanMPhm wants to merge 1 commit intonerc-project:mainfrom
QuanMPhm:ops_1391/final
Open

Allow dynamic quota creation and removal#287
QuanMPhm wants to merge 1 commit intonerc-project:mainfrom
QuanMPhm:ops_1391/final

Conversation

@QuanMPhm
Copy link
Contributor

@QuanMPhm QuanMPhm commented Jan 21, 2026 •
edited
Loading

Closes nerc-project/operations#1391. This is how I would suggest to review this PR.

Two CLI commands have been added, add_quota_to_resource.py and remove_quota_from_resource.py. I would suggest understanding those two commands first. These commands allow us to dynamically add/remove quotas instead of having them hard-coded as they are currently done. These commands don't impact the quota objects in the clusters, nor the quota attributes in allocations. Their full impact is illustrated when used within the typical user workflow, or in tandem with validate_allocations.py. I would now suggest checking the changes to functional/openshift/test_allocations.py to see the full implications of this PR. The other functional test cases only contain minor changes.

Afterwards, tasks.py, validate_allocations.py, and the allocator base and subclasses should be reviewed. They are the main consumers of quota information. All other changes relatively minor.

Follow-up issues to write based on these comments:
openshift vm quotas
idempotency for register_default_quotas
storage billing
Clean-up hard-coded attributes

@QuanMPhm QuanMPhm marked this pull request as draft January 21, 2026 18:05
@QuanMPhm QuanMPhm force-pushed the ops_1391/final branch 6 times, most recently from b3c58d8 to 35273aa Compare January 29, 2026 17:08
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Jan 29, 2026

@knikolla I addressed all your suggestions on Slack except one:

To migrate the display name of an attribute
Before, since the attributes were stored in code, the migrations were also stored in code
Now, since the adding of new quota is a command, migrating the display name of an attribute should also be a command.

May I ask that I implement this feature in a subsequent PR, to prevent this PR from bloating even more? If not, I will implement this after I receive answers for my questions above.

@joachimweyl
Copy link

joachimweyl commented Jan 29, 2026

What is the impact of this omission?

@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Jan 29, 2026 •
edited
Loading

@joachimweyl The impact will be that to change the display names of attributes (the names that users will see in the Coldfront UI, i.e OpenShift Limit on CPU Quota) will be a bit inconvenient. An admin will have to do some manual renaming in Coldfront. Still doable, but not in a way that's quick and programmatic. We ideally want a CLI command that makes renaming easier, but I didn't want this PR to take too long to review because of the February maintenance.

@joachimweyl
Copy link

joachimweyl commented Jan 30, 2026 •
edited
Loading

Makes sense to me.

@QuanMPhm QuanMPhm marked this pull request as ready for review February 4, 2026 18:24
knikolla
knikolla reviewed Feb 5, 2026
View reviewed changes
knikolla
knikolla reviewed Feb 5, 2026
View reviewed changes
knikolla
knikolla reviewed Feb 5, 2026
View reviewed changes
knikolla
knikolla requested changes Feb 5, 2026
View reviewed changes
naved001
naved001 reviewed Feb 5, 2026
View reviewed changes
Copy link
Contributor

@naved001 naved001 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @QuanMPhm! Some basic questions in there as I try to refresh my memory of coldfront. Will do another pass.

naved001
naved001 reviewed Feb 11, 2026
View reviewed changes
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Feb 13, 2026 •
edited
Loading

@knikolla @naved001 A one-time command migrate_resource_quota.py has been added.

EDIT: Sorry I've missed some comments. I get to work on them

@QuanMPhm QuanMPhm requested a review from knikolla February 13, 2026 16:49
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Feb 25, 2026 •
edited
Loading

@knikolla @naved001 This PR is waiting on review. This is my last question

naved001
naved001 reviewed Feb 26, 2026
View reviewed changes
knikolla
knikolla reviewed Feb 26, 2026
View reviewed changes
Copy link
Collaborator

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! I think this will be ready with a few minor iterations mostly related to fetching rate data from invoicing and some polish here and there.

I'll do a deeper pass in the afternoon but I don't expect anything big to jump out. Again, good work!

@QuanMPhm QuanMPhm force-pushed the ops_1391/final branch 4 times, most recently from 12789b2 to 29ff0dc Compare February 27, 2026 15:25
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Feb 27, 2026

@knikolla @naved001 I have responded to all comments so far. In response to this comment, I decided to use register_default_quotas in the test cases to remove some redundant code.

knikolla
knikolla reviewed Mar 3, 2026
View reviewed changes
Copy link
Collaborator

@knikolla knikolla left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, some questions.

@knikolla knikolla requested a review from Copilot March 3, 2026 19:50
@knikolla
Copy link
Collaborator

knikolla commented Mar 3, 2026

Adding copilot to check if I missed something. If there's valid feedback I will tag you on the relevant comments.

Copilot AI reviewed Mar 3, 2026
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR moves quota definitions from hard-coded mappings into per-resource, dynamically managed quota specs stored on the resource, and updates allocators/tasks/commands/tests to consume those specs. It adds management commands to register defaults and to add/remove quotas on a resource so quotas can evolve without code changes.

Changes:

  • Introduce QuotaSpec/QuotaSpecs models and load quota specs from a resource attribute (Available Quota Resources) in allocators.
  • Add management commands to register default quotas and to add/remove quota specs on a resource.
  • Refactor OpenShift/OpenStack allocators, tasks.activate_allocation, allocation validation, and storage billing to use resource-defined quota specs; update functional/unit tests accordingly.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 9 comments.

Show a summary per file
File Description
src/coldfront_plugin_cloud/tests/unit/test_usage_models.py Update imports for moved usage_models.
src/coldfront_plugin_cloud/tests/unit/test_register_default_quotas.py New unit tests for register_default_quotas behavior/idempotency.
src/coldfront_plugin_cloud/tests/unit/test_fetch_daily_billable_usage.py Update imports for moved usage_models.
src/coldfront_plugin_cloud/tests/unit/test_calculate_quota_unit_hours.py Ensure test resources have quota specs via add_quota_to_resource.
src/coldfront_plugin_cloud/tests/unit/openshift/test_rbac.py Switch to shared unit OpenShift base allocator.
src/coldfront_plugin_cloud/tests/unit/openshift/base.py Add mock allocator wrapper to avoid real allocator initialization dependencies.
src/coldfront_plugin_cloud/tests/functional/openstack/test_allocation.py Adjust tests to rely on registered default quotas and resource quota specs.
src/coldfront_plugin_cloud/tests/functional/openshift_vm/test_allocation.py Register defaults and customize VM GPU quotas via add/remove quota commands.
src/coldfront_plugin_cloud/tests/functional/openshift/test_allocation.py Register defaults and expand tests around adding/removing quotas dynamically.
src/coldfront_plugin_cloud/tests/functional/esi/test_allocations.py Set up ESI quota specs dynamically in test setup.
src/coldfront_plugin_cloud/tests/base.py Update OpenShift resource creation signature (internal_name) and remove IBM storage flag.
src/coldfront_plugin_cloud/tasks.py Replace hard-coded quota math with QuotaSpec-driven computation.
src/coldfront_plugin_cloud/openstack.py Replace static quota mappings with resource-defined quota specs grouped by service.
src/coldfront_plugin_cloud/openshift_vm.py Remove VM-specific hard-coded quota mapping in favor of resource-defined specs.
src/coldfront_plugin_cloud/openshift.py Remove hard-coded quota mapping; format quotas via resource-defined specs.
src/coldfront_plugin_cloud/models/usage_models.py New module for pydantic usage/charges models (moved from old location).
src/coldfront_plugin_cloud/models/quota_models.py New pydantic models for quota specs and validation.
src/coldfront_plugin_cloud/management/commands/validate_allocations.py Validate allocations based on per-resource quota specs rather than hard-coded mappings.
src/coldfront_plugin_cloud/management/commands/remove_quota_from_resource.py New command to remove a quota spec from a resource.
src/coldfront_plugin_cloud/management/commands/register_default_quotas.py New command to populate default quota specs onto OpenShift/OpenStack resources.
src/coldfront_plugin_cloud/management/commands/fetch_daily_billable_usage.py Update imports for moved usage_models.
src/coldfront_plugin_cloud/management/commands/calculate_storage_gb_hours.py Drive storage billing from resource quota specs of type storage.
src/coldfront_plugin_cloud/management/commands/add_quota_to_resource.py New command to add a quota spec to a resource and create its allocation attribute type.
src/coldfront_plugin_cloud/management/commands/add_openshift_resource.py Remove IBM storage attribute wiring; add internal cluster name support.
src/coldfront_plugin_cloud/esi.py Remove ESI hard-coded quota mapping in favor of inherited dynamic behavior.
src/coldfront_plugin_cloud/base.py Load QuotaSpecs from resource attribute in allocator initialization.
src/coldfront_plugin_cloud/attributes.py Replace IBM storage availability attribute with Available Quota Resources; adjust registered allocation quota attributes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@QuanMPhm
Allow dynamic quota creation and removal
8703161 
Allocation quota attributes are no longer hardcoded in `attributes.py`
and instead are created dynamically based on the quotas defined for each
resource. This eliminates the need to modify code and
restart the Coldfront service to update quotas.

3 new commands are added to implement this feature:
- `add_quota_to_resource`: Adds a quota to a resource.
- `remove_quota_from_resource`: Removes a quota from a resource.
- `register_default_quotas`: Registers default quotas for all
resources based a predetermined list

These commands will interact with a new resource attribute
`Available Quota Resources`, which stores a list of all quota resources
that can be applied to a resource. When a quota is added or removed
using the above commands, this attribute will be updated accordingly.

Refactored various files to support dynamic quotas
Updated functional and unit tests
@QuanMPhm
Copy link
Contributor Author

QuanMPhm commented Mar 4, 2026

Updated commit message

@QuanMPhm QuanMPhm requested a review from knikolla March 4, 2026 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@knikolla knikolla knikolla approved these changes

@naved001 naved001 naved001 approved these changes

@jtriley jtriley Awaiting requested review from jtriley

@Milstein Milstein Awaiting requested review from Milstein

@joachimweyl joachimweyl Awaiting requested review from joachimweyl

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Update Storage types in ColdFront

5 participants

@QuanMPhm @joachimweyl @knikolla @naved001